fix: apply single_distinct_to_groupby to aliased DataFrame API aggregates#23403
Open
fangchenli wants to merge 2 commits into
Open
fix: apply single_distinct_to_groupby to aliased DataFrame API aggregates#23403fangchenli wants to merge 2 commits into
fangchenli wants to merge 2 commits into
Conversation
fangchenli
force-pushed
the
fix/single-distinct-dataframe-alias
branch
2 times, most recently
from
1aa3e4c to
811088a
Compare
Contributor
|
@fangchenli if you could fix the failing tests, I will review soon. |
fangchenli
force-pushed
the
fix/single-distinct-dataframe-alias
branch
from
2712985 to
c388b94
Compare
…ates The SingleDistinctToGroupBy rule rewrites grouped count(DISTINCT col) into a nested double aggregate, but only fired for SQL-planned plans. The DataFrame API applies the output alias directly to the aggregate expression, so aggr_expr holds Expr::Alias(AggregateFunction); is_single_distinct_agg only matched a bare Expr::AggregateFunction and the rule never fired, leaving the DataFrame path materially slower than the equivalent SQL. Peel all top-level Expr::Alias wrappers in both the matcher and the rewrite (aliases nested inside sub-expressions are left untouched). Output column names are preserved because the rule already rebuilds the outer projection from the original aggregate's schema. Mirrors the fix for the same bug class in apache#21411 / apache#21438. Also skip the rewrite when a DISTINCT argument is a constant. Such an argument would become a constant GROUP BY key that eliminate_group_by_constant collapses into a global aggregate, which emits a row even on empty input and produces wrong results (e.g. MIN(DISTINCT 37) over empty input returned 37, not NULL). This is a latent bug the alias fix would otherwise expose more widely; the rewrite has no benefit for a constant argument anyway. Closes apache#23401. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fangchenli
force-pushed
the
fix/single-distinct-dataframe-alias
branch
from
c388b94 to
7975288
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
single_distinct_to_groupbynot applied to DataFrame API count(DISTINCT) #23401.Rationale for this change
The SingleDistinctToGroupBy optimizer rule fires for the SQL planner but not for the logically-equivalent plan built by the DataFrame API, because the two front-ends place the output alias differently:
is_single_distinct_agg only matched a bare Expr::AggregateFunction, so the aliased shape fell through and the rule never fired. As a result the DataFrame API path was materially slower than the identical SQL query.
What changes are included in this PR?
Are these changes tested?
Yes, unit tests added.
Are there any user-facing changes?
No behavior change, except performance improvement.